support query-based terms lookup queries in DLS by rursprung · Pull Request #6244 · opensearch-project/security

rursprung · 2026-06-23T15:40:30Z

Description

OpenSearch introduced the possibility to use a query rather than just
an id in Terms Lookup Queries in opensearch-project/OpenSearch#18195 in release 3.2.0.
However, support for this was never added to the security plugin for
DLS/FLS.

When trying to use query based TLQ in a DLS this failed for two
reasons:

the visible failure is an NPE because DlsFilterLevelActionHandler
unconditionally dereferenced termsLookup().id(), which returns
null for query-based lookups.
additionally, the DocumentAllowList privilege bypass only handled
GetRequest, but query-based TLQ internally resolves via
SearchRequest and additionally uses a GetSettingsRequest, causing
the privileges evaluator to block the lookup even after the NPE is
fixed.

This commit adds proper support for query based TLQ in DLS, both for
the old and the new evaluator, by allowlisting a wildcard entry for the
whole index in question rather than a single document in case query is
used. additionally, SearchRequest and GetSettingsRequest are now
also supported in DocumentAllowList#isAllowed.

Furthermore, the legacy
PrivilegesEvaluatorImpl#checkDocAllowListHeader now delegates to
DocumentAllowList#isAllowed since the old method body was a
hand-inlined copy of the same logic (parse header, check request);
delegating removes the duplication and picks up SearchRequest support
without repeating the new code.

Header parsing in DocumentAllowList#isAllowed is now delegated to
DocumentAllowList#get to centralise error handling.

The wildcard allowlist pattern is already established for Dashboards
multi-tenancy and is scoped to the request's thread context, so it
cannot grant persistent or write access beyond the TLQ resolution.
Existing code with the wildcard has been migrated to use the new
constant to make it easier to find it.

additionally, DlsTermLookupQueryV4EvaluatorTest has been added (in a separate commit) to run the tests from DlsTermLookupQueryTest also against the new evaluator (this unearthed that GetSettingsRequest had to be whitelisted as well; the old evaluator didn't block this!).

Issues Resolved

fixes #6243

Do these changes introduce new permission(s) to be displayed in the static dropdown on the front-end?
no

Testing

unit & integration testing + manual testing

Check List

New functionality includes testing
New functionality has been documented - shouldn't be needed since everyone will just expect this to work?
New Roles/Permissions have a corresponding security dashboards plugin PR
API changes companion pull request created
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-actions · 2026-06-23T15:42:03Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 6a67442.

Path	Line	Severity	Description
src/main/java/org/opensearch/security/privileges/DocumentAllowList.java	73	medium	The isAllowed() bypass is extended to cover SearchRequest and GetSettingsRequest in addition to the previous GetRequest-only scope. If the OPENDISTRO_SECURITY_DOC_ALLOWLIST_HEADER can be injected or forged (e.g., insufficient stripping of incoming transport headers), an attacker could trigger DLS bypass for arbitrary search operations. The change is contextually justified for query-based TLQ, but significantly widens the privilege-bypass surface and warrants verification that the header is always stripped from external requests.
src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java	524	medium	When termsLookup().id() is null (query-based TLQ), ANY_DOCUMENT_ID ("*") is added to the document allowlist for the lookup index. This wildcard entry causes isIndicesAllowlisted() to return true for any SearchRequest against that index, bypassing DLS entirely for the lookup index. If the null-id path can be reached by a crafted TermsQueryBuilder that is not a legitimate internal TLQ (e.g., user-supplied query reaching this code path), it could allow unbounded access to the lookup index.
src/test/resources/dlsfls/roles_tlq.yml	20	low	The new test role os_dls_tlq_query_lookup is granted cluster_permissions: ["*"], giving all cluster-level permissions to its mapped users. This is overly broad even for a test role; a misconfigured test environment importing this config into a non-test cluster would expose all cluster APIs to this role. Minimal required permissions should be specified instead.

The table above displays the top 10 most important findings.

Total: 3 | Critical: 0 | High: 0 | Medium: 2 | Low: 1

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

github-actions · 2026-06-23T15:42:48Z

PR Reviewer Guide 🔍

(Review updated until commit `6a67442`)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review Overly Restrictive All-Indices Check `isIndicesAllowlisted` requires every index in a `SearchRequest`/`GetSettingsRequest` to have a wildcard allowlist entry. If a TLQ resolves internally against its lookup index but the request also carries additional non-allowlisted indices (e.g., an aliased or expanded target), the whole request will be denied. Also, `searchRequest.indices()` returns the pre-resolution names (patterns/aliases), while `DlsFilterLevelActionHandler` allowlists the concrete `termsLookup().index()` name - these may not match for aliases, causing lookups to fail. Worth confirming with an alias/pattern scenario. private static boolean isIndicesAllowlisted(DocumentAllowList documentAllowList, String[] indices) { if (indices == null \|\| indices.length == 0) { return false; } for (String index : indices) { if (index == null \|\| !documentAllowList.isAllowed(index, ANY_DOCUMENT_ID)) { return false; } } return true; } Ineffective Ignore Annotation The `@Ignore` annotation is placed on the overriding `testMGet_1337` method, but the method still calls `super.testMGet_1337()`. Since `@Ignore` prevents JUnit from executing the test, the annotation works, but the method body is dead code. More importantly, if the intent was to document expected-failure behavior, this silently skips the test rather than asserting the expected 403. Consider asserting the expected V4 behavior explicitly instead of ignoring. @Override @Test @Ignore("expected to fail with V4 evaluator") public void testMGet_1337() throws Exception { // V4 evaluator does not silently strip unauthorized indices from explicit _mget requests. // Legacy uses DNFOF_MATCHER on "indices:data/read/*" to reduce indices; V4 only reduces // for wildcard/pattern requests or ignore_unavailable=true. Concrete unauthorized index -> 403. super.testMGet_1337(); }

github-actions · 2026-06-23T15:43:09Z

PR Code Suggestions ✨

Latest suggestions up to 6a67442

Explore these optional code suggestions:

Category	Suggestion	Impact
Security	Restrict search bypass to single-index requests `isIndicesAllowlisted` requires ALL indices in the request to be allowlisted. A malicious/legitimate SearchRequest could include the TLQ lookup index plus additional unauthorized indices, and this method will simply return `false`, but the caller then relies on normal privilege evaluation. More concerning: if a SearchRequest targets only allowlisted indices with a wildcard entry, the caller bypasses privilege checks entirely. Ensure the check considers whether the request is an internal TLQ sub-request (e.g., by verifying request context or single-index constraint) rather than granting bypass for any SearchRequest whose indices happen to match. src/main/java/org/opensearch/security/privileges/DocumentAllowList.java [71-76] } else if (request instanceof SearchRequest searchRequest) { - if (isIndicesAllowlisted(documentAllowList, searchRequest.indices())) { + // Only bypass for single-index TLQ sub-requests to avoid granting broad search access + // whenever an allowlisted index appears among multiple targeted indices. + String[] indices = searchRequest.indices(); + if (indices != null && indices.length == 1 && isIndicesAllowlisted(documentAllowList, indices)) { log.debug("Request {} is allowed by {}", request, documentAllowList); return true; } return false; } else if (request instanceof GetSettingsRequest getSettingsRequest) { Suggestion importance[1-10]: 6 __ Why: Raises a valid security concern about broadening bypass semantics for SearchRequests, though the actual exploitability depends on how allowlist entries are populated and whether the caller relies on normal privilege evaluation afterward. The suggestion is speculative but worth considering.	Low
Security	Wildcard TLQ entry may over-authorize searches When `lookupId` is null (query-based TLQ), adding a wildcard entry `(lookupIndex, "*")` grants any SearchRequest whose indices match `lookupIndex` a full bypass of privilege evaluation via `DocumentAllowList.isAllowed`. This effectively expands the user's read access to the entire lookup index for any subsequent search-shaped request in the same thread context. Consider a narrower marker (e.g., a distinct sentinel or a scoped flag) that only permits the internal TLQ resolution and is not honored for arbitrary user SearchRequests. src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java [518-528] for (QueryBuilder queryBuilder : queryBuilders) { TermsQueryBuilder termsQueryBuilder = (TermsQueryBuilder) queryBuilder; final var lookupIndex = termsQueryBuilder.termsLookup().index(); final var lookupId = termsQueryBuilder.termsLookup().id(); if (lookupId != null) { documentAllowlist.add(lookupIndex, lookupId); } else { + // Query-based TLQ: allow only the internal search sub-request against the lookup index. documentAllowlist.add(lookupIndex, DocumentAllowList.ANY_DOCUMENT_ID); } } Suggestion importance[1-10]: 5 __ Why: Highlights a potential security concern with wildcard allowlist entries but does not provide a concrete alternative (the improved_code is essentially identical to existing_code with just a comment added).	Low
General	Avoid calling super in ignored override test The overridden test still invokes `super.testMGet_1337()`, so despite `@Ignore` the parent's assertions will run if the annotation is ever removed and currently the body is dead code. If the V4 behavior is expected to differ (403 instead of silent strip), replace the super call with an assertion that validates the expected V4 outcome; otherwise leave only the `@Ignore` with no super invocation to avoid future confusion. src/test/java/org/opensearch/security/dlic/dlsfls/DlsTermLookupQueryV4EvaluatorTest.java [31-38] @Override @Test -@Ignore("expected to fail with V4 evaluator") +@Ignore("expected to fail with V4 evaluator - concrete unauthorized index yields 403 under V4") public void testMGet_1337() throws Exception { - // V4 evaluator does not silently strip unauthorized indices from explicit _mget requests. - // Legacy uses DNFOF_MATCHER on "indices:data/read/*" to reduce indices; V4 only reduces - // for wildcard/pattern requests or ignore_unavailable=true. Concrete unauthorized index -> 403. - super.testMGet_1337(); + // Intentionally not invoking super: V4 evaluator returns 403 for concrete unauthorized indices, + // whereas legacy silently strips them via DNFOF_MATCHER. } Suggestion importance[1-10]: 3 __ Why: Minor style improvement; the test is `@Ignore`d so the super call has no effect, but removing it makes intent clearer.	Low

Previous suggestions

Suggestions up to commit d84e39e

Category	Suggestion	Impact
General	Verify all-indices allowlist semantics for search `isIndicesAllowlisted` requires every index in the request to be allowlisted. For a query-based TLQ, the inner `SearchRequest` typically targets only the single lookup index, but if the request resolves multiple indices (aliases, wildcards expanded elsewhere) any not-in-allowlist index will cause a `false` result and reject the entire lookup. Consider whether "all indices must be allowlisted" is truly correct here, or if the lookup-index-only check should be used to avoid false negatives. src/main/java/org/opensearch/security/privileges/DocumentAllowList.java [71-77] } else if (request instanceof SearchRequest searchRequest) { + // Note: For TLQ-generated searches, the request should target only the lookup index. + // If future callers issue multi-index searches with the allowlist header, revisit this policy. if (isIndicesAllowlisted(documentAllowList, searchRequest.indices())) { log.debug("Request {} is allowed by {}", request, documentAllowList); return true; } return false; } else if (request instanceof GetSettingsRequest getSettingsRequest) { Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid concern about the strict all-indices allowlist semantics but only adds a comment without changing behavior. It's a mostly informational note rather than a concrete fix.	Low
General	Avoid invoking known-failing super method The `@Ignore` annotation is placed after `@Test`, but the method still calls `super.testMGet_1337()` which is documented as expected to fail. If JUnit's ignore precedence changes or the annotation is removed accidentally, this test will fail. Consider making the body assert the V4-specific expected behavior (e.g., 403 response) instead of delegating to a superclass method known to fail. src/test/java/org/opensearch/security/dlic/dlsfls/DlsTermLookupQueryV4EvaluatorTest.java [31-38] @Override @Test -@Ignore("expected to fail with V4 evaluator") +@Ignore("V4 evaluator returns 403 for _mget with concrete unauthorized index; behavior tested separately") public void testMGet_1337() throws Exception { - // V4 evaluator does not silently strip unauthorized indices from explicit _mget requests. - // Legacy uses DNFOF_MATCHER on "indices:data/read/*" to reduce indices; V4 only reduces - // for wildcard/pattern requests or ignore_unavailable=true. Concrete unauthorized index -> 403. - super.testMGet_1337(); + // Intentionally empty: V4 evaluator does not silently strip unauthorized indices from + // explicit _mget requests, so the legacy test expectations do not apply here. } Suggestion importance[1-10]: 3 __ Why: Since the test is `@Ignore`d, JUnit will not execute the body, making the call to `super.testMGet_1337()` effectively dead code. The suggestion is a minor code quality improvement rather than a functional fix.	Low

Suggestions up to commit dd84988

Category	Suggestion	Impact
Security	Validate lookup index before allowlisting When `lookupId` is null (query-based TLQ), the entire index is allowlisted with `ANY_DOCUMENT_ID`, which grants the user implicit read access to all documents in `lookupIndex` via internal sub-requests. Consider validating that `lookupIndex` is not user-controllable in a way that could escalate privileges, or scope the allowlist more narrowly (e.g., by also tracking the query) to limit the bypass surface. src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java [520-527] final var lookupIndex = termsQueryBuilder.termsLookup().index(); final var lookupId = termsQueryBuilder.termsLookup().id(); +if (lookupIndex == null \|\| lookupIndex.isEmpty()) { + continue; +} if (lookupId != null) { documentAllowlist.add(lookupIndex, lookupId); } else { documentAllowlist.add(lookupIndex, DocumentAllowList.ANY_DOCUMENT_ID); } Suggestion importance[1-10]: 4 __ Why: Adding a null/empty check for `lookupIndex` is a reasonable defensive measure, but `termsLookup().index()` is typically required and non-null in valid TLQ definitions. The suggestion provides marginal hardening.	Low
Security	Guard against empty indices in allowlist check `isIndicesAllowlisted` returns false when `indices` is null or empty, which means a `SearchRequest` without any indices specified (interpreted as "all indices" by OpenSearch) would be incorrectly allowlisted only if it explicitly listed the indices. Worse, an attacker could craft a SearchRequest with no indices to broaden scope. Ensure that an empty indices array does not lead to unintended bypass and verify whether `SearchRequest.indices()` semantics here require explicit, non-empty allowlisted indices (current behavior returns false, which is safer—double-check this matches the TLQ resolution path actually invoked). src/main/java/org/opensearch/security/privileges/DocumentAllowList.java [71-83] } else if (request instanceof SearchRequest searchRequest) { - if (isIndicesAllowlisted(documentAllowList, searchRequest.indices())) { + String[] indices = searchRequest.indices(); + if (indices != null && indices.length > 0 && isIndicesAllowlisted(documentAllowList, indices)) { log.debug("Request {} is allowed by {}", request, documentAllowList); return true; } return false; } else if (request instanceof GetSettingsRequest getSettingsRequest) { - if (isIndicesAllowlisted(documentAllowList, getSettingsRequest.indices())) { + String[] indices = getSettingsRequest.indices(); + if (indices != null && indices.length > 0 && isIndicesAllowlisted(documentAllowList, indices)) { log.debug("Request {} is allowed by {}", request, documentAllowList); return true; } return false; } Suggestion importance[1-10]: 3 __ Why: The `isIndicesAllowlisted` helper already returns false for null or empty indices arrays, so the suggested change is redundant. The score reflects only a minor defensive coding improvement.	Low
General	Avoid invoking failing super in ignored test A test that is expected to fail under V4 should either assert the new (403) behavior explicitly or be removed/ignored permanently with rationale. Calling `super.testMGet_1337()` inside an `@Ignore`d method is dead code; if the goal is to document the V4 behavior, replace with an actual test asserting the 403 response so regressions in V4 are detected. src/test/java/org/opensearch/security/dlic/dlsfls/DlsTermLookupQueryV4EvaluatorTest.java [31-38] @Override @Test -@Ignore("expected to fail with V4 evaluator") +@Ignore("V4 evaluator returns 403 for explicit _mget on unauthorized index; legacy behavior covered by parent test") public void testMGet_1337() throws Exception { - // V4 evaluator does not silently strip unauthorized indices from explicit _mget requests. - // Legacy uses DNFOF_MATCHER on "indices:data/read/*" to reduce indices; V4 only reduces - // for wildcard/pattern requests or ignore_unavailable=true. Concrete unauthorized index -> 403. - super.testMGet_1337(); + // Intentionally not invoking super; legacy DNFOF behavior does not apply to V4. } Suggestion importance[1-10]: 3 __ Why: Since the test is annotated with `@Ignore`, the body never executes, so calling `super.testMGet_1337()` is harmless dead code. The suggestion is a minor cleanup with low impact.	Low

Suggestions up to commit 55eb985

Category	Suggestion	Impact
Security	Guard against wildcard-allowlisting protected indices Granting a wildcard `ANY_DOCUMENT_ID` allowlist on the lookup index whenever `id` is null means any query-based TLQ effectively lifts DLS restrictions on that entire index for the duration of the request. Ensure the lookup index is not the same as the index being queried (which would silently lift DLS on the user's data index); add a guard or document this constraint to avoid privilege escalation. src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java [520-527] final var lookupIndex = termsQueryBuilder.termsLookup().index(); final var lookupId = termsQueryBuilder.termsLookup().id(); if (lookupId != null) { documentAllowlist.add(lookupIndex, lookupId); } else { + // Query-based TLQ: lookup index gets wildcard allowlist. Ensure lookup index is distinct + // from any index whose DLS should remain enforced. documentAllowlist.add(lookupIndex, DocumentAllowList.ANY_DOCUMENT_ID); } Suggestion importance[1-10]: 5 __ Why: The suggestion raises a legitimate security concern about wildcard allowlisting on a lookup index, but the improved_code only adds a comment without adding an actual guard, so the impact is mostly documentation.	Low
General	Clarify handling of empty indices array `isIndicesAllowlisted` returns false when `indices` is null/empty, but a `SearchRequest` with no explicit indices typically targets all indices (`_all`). This may incorrectly deny a sub-search that the security layer expanded earlier; conversely, treating empty as "match all" must not bypass checks. Verify the expected behavior and at minimum log/handle the empty-indices case explicitly so query-based TLQ sub-searches built without index names are not silently rejected. src/main/java/org/opensearch/security/privileges/DocumentAllowList.java [71-76] } else if (request instanceof SearchRequest searchRequest) { - if (isIndicesAllowlisted(documentAllowList, searchRequest.indices())) { + String[] indices = searchRequest.indices(); + if (indices == null \|\| indices.length == 0) { + log.debug("SearchRequest with no indices not allowlisted: {}", request); + return false; + } + if (isIndicesAllowlisted(documentAllowList, indices)) { log.debug("Request {} is allowed by {}", request, documentAllowList); return true; } return false; } else if (request instanceof GetSettingsRequest getSettingsRequest) { Suggestion importance[1-10]: 4 __ Why: The suggestion raises a potentially valid concern about empty `indices` arrays in `SearchRequest`, but the improved code is functionally equivalent to existing code (both return false) and just adds a debug log, making the impact minimal.	Low
General	Assert expected V4 behavior instead of ignoring The `@Ignore` annotation must come before `@Test` (or replace it) and JUnit 4's `@Ignore` order matters less, but invoking `super.testMGet_1337()` inside a method annotated `@Ignore` is fine — however, if the intent is to skip, ensure the method body is not executed. Currently `@Ignore` will skip it, but the comment says "expected to fail"; consider using an assertion of the expected failure (e.g., expect a 403) so regressions are detected, rather than silently ignoring. src/test/java/org/opensearch/security/dlic/dlsfls/DlsTermLookupQueryV4EvaluatorTest.java [31-38] @Override @Test -@Ignore("expected to fail with V4 evaluator") public void testMGet_1337() throws Exception { // V4 evaluator does not silently strip unauthorized indices from explicit _mget requests. - // Legacy uses DNFOF_MATCHER on "indices:data/read/*" to reduce indices; V4 only reduces - // for wildcard/pattern requests or ignore_unavailable=true. Concrete unauthorized index -> 403. - super.testMGet_1337(); + // Concrete unauthorized index -> 403 expected. Override to assert this behavior instead of + // running the legacy expectations. + setupWithTlqSecurityConfig(); + // ... assert 403 / expected V4 behavior here } Suggestion importance[1-10]: 3 __ Why: The suggestion is somewhat valid in noting that `@Ignore` skips the test silently, but the `improved_code` is incomplete (just a placeholder) and the existing approach is a reasonable temporary measure documented with comments.	Low

Suggestions up to commit a8dcab5

Category	Suggestion	Impact
Security	Restrict wildcard fallback to query-based lookups Adding a wildcard-document allowlist entry whenever `lookupId` is null grants bypass to the entire lookup index for any TLQ that omits an id, including ones that may not actually use a query (could result in a broader bypass than intended). Verify a `query` is actually present on the `TermsLookup` before falling back to wildcard, otherwise restrict to id-based entries to avoid unintended index-wide privilege escalation. src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java [520-527] final var lookupIndex = termsQueryBuilder.termsLookup().index(); final var lookupId = termsQueryBuilder.termsLookup().id(); +final var lookupQuery = termsQueryBuilder.termsLookup().query(); if (lookupId != null) { documentAllowlist.add(lookupIndex, lookupId); -} else { +} else if (lookupQuery != null) { documentAllowlist.add(lookupIndex, DocumentAllowList.WILDCARD_DOCUMENT_ID); } Suggestion importance[1-10]: 6 __ Why: Reasonable defensive suggestion to verify a `query` exists on the `TermsLookup` before adding a wildcard allowlist entry, though the surrounding filter already requires `termsLookup() != null` so the practical risk is lower.	Low
Possible issue	Handle alias/wildcard indices in allowlist check `SearchRequest` and `GetSettingsRequest` accept index patterns/aliases (e.g. wildcards like `user_*`), but `isIndicesAllowlisted` does exact-string matching against the allowlist. A TLQ-driven sub-request that uses an alias or pattern instead of a concrete index name will fail the bypass check. Resolve indices against cluster state (or restrict allowed bypass to concrete index names only) before comparing, otherwise legitimate query-based TLQ lookups against aliases will be denied. src/main/java/org/opensearch/security/privileges/DocumentAllowList.java [71-83] } else if (request instanceof SearchRequest searchRequest) { + // Note: only concrete index names are supported; aliases/patterns must be resolved by caller if (isIndicesAllowlisted(documentAllowList, searchRequest.indices())) { log.debug("Request {} is allowed by {}", request, documentAllowList); return true; } return false; } else if (request instanceof GetSettingsRequest getSettingsRequest) { if (isIndicesAllowlisted(documentAllowList, getSettingsRequest.indices())) { log.debug("Request {} is allowed by {}", request, documentAllowList); return true; } return false; } Suggestion importance[1-10]: 5 __ Why: Raises a legitimate concern that aliases/patterns are not resolved before allowlist matching, but the `improved_code` only adds a comment and does not actually fix the issue, limiting its impact.	Low
General	Remove dead super-call in ignored test Calling `super.testMGet_1337()` inside an `@Ignore`d test is dead code and misleading; with `@Ignore` the body never executes. Either drop the override entirely (so the parent test runs and passes/fails on its own) or, if it must be skipped under V4, leave only the `@Ignore` with no body and a comment, to make intent clear. src/test/java/org/opensearch/security/dlic/dlsfls/DlsTermLookupQueryV4EvaluatorTest.java [31-38] @Override @Test -@Ignore("expected to fail with V4 evaluator") -public void testMGet_1337() throws Exception { - // V4 evaluator does not silently strip unauthorized indices from explicit _mget requests. - // Legacy uses DNFOF_MATCHER on "indices:data/read/*" to reduce indices; V4 only reduces - // for wildcard/pattern requests or ignore_unavailable=true. Concrete unauthorized index -> 403. - super.testMGet_1337(); +@Ignore("V4 evaluator does not silently strip unauthorized indices from explicit _mget requests.") +public void testMGet_1337() { + // Intentionally skipped under V4 evaluator. } Suggestion importance[1-10]: 2 __ Why: Minor cleanup of misleading dead code in an ignored test; has no functional impact.	Low

Suggestions up to commit ab9eb03

Category	Suggestion	Impact
Possible issue	Handle wildcard index expressions in allowlist check `SearchRequest.indices()` may contain wildcards/patterns/aliases (e.g., , `tlq`) that won't literally match an entry like `my_index` in the allowlist, causing legitimate query-based TLQ sub-search requests on resolved index names to be rejected. Consider matching using pattern semantics or only allowlisting based on the resolved concrete indices, otherwise TLQ may break for any non-literal index expression. src/main/java/org/opensearch/security/privileges/DocumentAllowList.java [71-76] +} else if (request instanceof SearchRequest searchRequest) { + if (isIndicesAllowlisted(documentAllowList, searchRequest.indices())) { + log.debug("Request {} is allowed by {}", request, documentAllowList); + return true; + } + return false; +} else if (request instanceof GetSettingsRequest getSettingsRequest) { - Suggestion importance[1-10]: 5 __ Why: Valid concern that wildcard/pattern index expressions in `SearchRequest.indices()` may not literally match allowlist entries, but the suggestion does not provide a concrete fix and the actual TLQ sub-requests typically use resolved concrete index names.	Low
General	Avoid invoking superclass body in ignored test The overridden test calls `super.testMGet_1337()` directly, which means if `@Ignore` is ever removed or the runner ignores it differently, the parent assertions will run unchanged and fail rather than asserting the expected V4 behavior (403). Either delete the body entirely (since `@Ignore` skips it) or replace it with assertions that capture the actual expected V4 behavior so the documented difference is properly verified. src/test/java/org/opensearch/security/dlic/dlsfls/DlsTermLookupQueryV4EvaluatorTest.java [31-38] @Override @Test -@Ignore("expected to fail with V4 evaluator") +@Ignore("expected to fail with V4 evaluator - see comment") public void testMGet_1337() throws Exception { // V4 evaluator does not silently strip unauthorized indices from explicit _mget requests. - // Legacy uses DNFOF_MATCHER on "indices:data/read/*" to reduce indices; V4 only reduces - // for wildcard/pattern requests or ignore_unavailable=true. Concrete unauthorized index -> 403. - super.testMGet_1337(); + // Concrete unauthorized index -> 403, unlike legacy DNFOF behavior. } Suggestion importance[1-10]: 4 __ Why: The `@Ignore` annotation prevents execution, so calling `super.testMGet_1337()` is harmless but slightly misleading. Minor cleanup with low impact.	Low
General	Guard against null lookup index When `lookupId` is null (query-based TLQ), the entire lookup index is allowlisted with a wildcard, which broadens access more than necessary—any document in the lookup index becomes readable via the bypass path during this request. Consider also verifying `lookupIndex` is non-null and document the wider trust scope, or restrict the bypass strictly to the search sub-request executing the lookup query. src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java [520-527] final var lookupIndex = termsQueryBuilder.termsLookup().index(); final var lookupId = termsQueryBuilder.termsLookup().id(); +if (lookupIndex == null) { + continue; +} if (lookupId != null) { documentAllowlist.add(lookupIndex, lookupId); } else { documentAllowlist.add(lookupIndex, DocumentAllowList.WILDCARD_DOCUMENT_ID); } Suggestion importance[1-10]: 3 __ Why: `termsLookup().index()` is generally required for valid terms lookup queries, so a null check is defensive but unlikely to be needed. Low impact.	Low

github-actions · 2026-06-24T14:14:08Z

Persistent review updated to latest commit ab9eb03

github-actions · 2026-06-24T14:19:52Z

Persistent review updated to latest commit a8dcab5

rursprung · 2026-06-24T14:24:51Z

the CI failure is a false reject (can't fetch maven snapshots). i've seen this happen a lot lately!

rursprung · 2026-06-24T14:26:18Z

CC @srikanthpadakanti (you implemented query support in TLQ in OpenSearch) and @jochenkressin & @nibix (you implemented/contributed the original TLQ-in-DLS support)

rursprung · 2026-06-25T07:38:08Z

from what i can see all CI failures are false rejects: they either failed due to not being able to fetch maven artefacts (i've seen that a lot now on PRs on this repo - what's going on here?) - or completely unrelated tests failing => probably nothing that i can help sort out here?

github-actions · 2026-06-29T11:55:21Z

Persistent review updated to latest commit 55eb985

github-actions · 2026-06-29T11:59:20Z

Persistent review updated to latest commit dd84988

codecov · 2026-06-29T12:31:44Z

Codecov Report

❌ Patch coverage is 80.00000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.20%. Comparing base (e8b9083) to head (6a67442).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...nsearch/security/privileges/DocumentAllowList.java	77.27%	1 Missing and 4 partials ⚠️
...search/security/configuration/DlsFlsValveImpl.java	50.00%	0 Missing and 1 partial ⚠️
...figuration/SecurityFlsDlsIndexSearcherWrapper.java	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6244      +/-   ##
==========================================
+ Coverage   75.10%   75.20%   +0.10%     
==========================================
  Files         451      451              
  Lines       29275    29276       +1     
  Branches     4416     4419       +3     
==========================================
+ Hits        21987    22018      +31     
+ Misses       5259     5212      -47     
- Partials     2029     2046      +17

Files with missing lines	Coverage Δ
...ity/configuration/DlsFilterLevelActionHandler.java	`57.58% <100.00%> (+0.77%)`	⬆️
...es/actionlevel/legacy/PrivilegesEvaluatorImpl.java	`87.01% <100.00%> (+1.11%)`	⬆️
...eges/actionlevel/legacy/PrivilegesInterceptor.java	`75.89% <100.00%> (ø)`
...tgen/DashboardsMultitenancySystemIndexHandler.java	`88.41% <100.00%> (ø)`
...search/security/configuration/DlsFlsValveImpl.java	`67.81% <50.00%> (ø)`
...figuration/SecurityFlsDlsIndexSearcherWrapper.java	`79.66% <0.00%> (ø)`
...nsearch/security/privileges/DocumentAllowList.java	`69.88% <77.27%> (+22.29%)`	⬆️

... and 11 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

nibix · 2026-06-29T18:04:17Z

@rursprung

Thank you for this and sorry for the late response; at the moment, I have quite a few different things going on.

rursprung · 2026-07-09T12:37:42Z

if we can't reach an understanding of how the whitelisting for the index targeted by the subquery should work before we hit the code freeze for 3.8.0 would it also be ok if i'd split this in two: this PR would only add support for running query-based TLQs in DLS and the authorization part would be a new issue. then it'd already be possible to use query-based TLQ in DLS as long as the user is granted read access to the index targeted by the subquery (a workable workaround which could be documented as such).

github-actions · 2026-07-09T16:22:47Z

Persistent review updated to latest commit d84e39e

rursprung · 2026-07-10T08:53:50Z

the CI failure seems to be an infrastructure issue. i've reported it on slack.
the push was just to rebase the PR.

reta · 2026-07-10T20:53:46Z

if we can't reach an understanding of how the whitelisting for the index targeted by the subquery should work before

I think I cleared everything out for myself, just to summarize for others, we do have:

the terms lookup by id -> just GetRequest, covered by document allow list with (index, id) pairs
the terms lookup by query -> settings + search requests, both need index wide permissions (since the ids are not available), covered by document allow list with (index, *) pairs

Thanks @rursprung for patience, I just have one minor comment to update the comments section, attributing settings call, LGTM otherwise!

OpenSearch introduced the possibility to use a `query` rather than just an `id` in Terms Lookup Queries in OpenSearch#18195 in release 3.2.0. However, support for this was never added to the security plugin for DLS/FLS. When trying to use `query` based TLQ in a DLS this failed for two reasons: - the visible failure is an NPE because `DlsFilterLevelActionHandler` unconditionally dereferenced `termsLookup().id()`, which returns `null` for query-based lookups. - additionally, the `DocumentAllowList` privilege bypass only handled `GetRequest`, but query-based TLQ internally resolves via `SearchRequest` and additionally uses a `GetSettingsRequest`, causing the privileges evaluator to block the lookup even after the NPE is fixed. This commit adds proper support for `query` based TLQ in DLS, both for the old and the new evaluator, by allowlisting a wildcard entry for the whole index in question rather than a single document in case `query` is used. additionally, `SearchRequest` and `GetSettingsRequest` are now also supported in `DocumentAllowList#isAllowed`. Furthermore, the legacy `PrivilegesEvaluatorImpl#checkDocAllowListHeader` now delegates to `DocumentAllowList#isAllowed` since the old method body was a hand-inlined copy of the same logic (parse header, check request); delegating removes the duplication and picks up `SearchRequest` support without repeating the new code. Header parsing in `DocumentAllowList#isAllowed` is now delegated to `DocumentAllowList#get` to centralise error handling. The wildcard allowlist pattern is already established for Dashboards multi-tenancy and is scoped to the request's thread context, so it cannot grant persistent or write access beyond the TLQ resolution. Existing code with the wildcard has been migrated to use the new constant to make it easier to find it. fixes opensearch-project#6243 Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Ursprung <Ralph.Ursprung@avaloq.com>

so far, the tests only used the legacy evaluator. add a new subclass to also test against V4 to ensure that this is also covered. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Ursprung <Ralph.Ursprung@avaloq.com>

rursprung · 2026-07-14T06:17:46Z

thanks @reta! i've incorporated your feedback & rebased the PR.

is there a need to raise a docs request for this at all? the docs already state that TLQ is supported in DLS and didn't state anywhere that query based TLQ wasn't supported (because this seems to have a been an oversight and not intentional in the first place) => i wouldn't know what to add to the docs beyond "this works too"?

github-actions · 2026-07-14T06:18:36Z

Persistent review updated to latest commit 6a67442

reta · 2026-07-14T12:43:26Z

is there a need to raise a docs request for this at all? the docs already state that TLQ is supported in DLS and didn't state anywhere that query based TLQ wasn't supported (because this seems to have a been an oversight and not intentional in the first place) => i wouldn't know what to add to the docs beyond "this works too"?

Thanks @rursprung , I think no need for doc changes, this is indeed an oversight (== bugfix), thanks!

rursprung · 2026-07-17T15:33:27Z

@reta: is there anything missing to get this merged before the code freeze? CI is happy and i see an approval on the PR (i'm not a maintainer, so i don't have the rights to hit merge)

reta · 2026-07-17T15:35:26Z

@reta: is there anything missing to get this merged before the code freeze? CI is happy and i see an approval on the PR (i'm not a maintainer, so i don't have the rights to hit merge)

Not from my side, @cwperks @nibix anything from your side? Thanks!

cwperks · 2026-07-17T16:00:58Z

@reta I don't have any objections, but I do think our coverage of FGAC for TLQs is lacking in general. I think this change looks sensible to me, and separately I would also like someone to take on adding FGAC focused tests for TLQs since you can "join" with another index. I certainly hope the subquery is being executed within the same context as the top level search.

cwperks · 2026-07-17T16:17:43Z

@rursprung additionally I would like to see coverage in scenarios where the TLQ is done on an index where the user has DLS/FLS restrictions. I haven't dug deep on this, but I don't think this repo has a lot of coverage in this feature that was added in 3.2.

rursprung · 2026-07-17T19:28:29Z

@rursprung additionally I would like to see coverage in scenarios where the TLQ is done on an index where the user has DLS/FLS restrictions. I haven't dug deep on this, but I don't think this repo has a lot of coverage in this feature that was added in 3.2.

@cwperks but that's in DlsTermLookupQueryTest - or what are you looking for? that's a whole class just testing TLQ in DLS. and i've extended it with tests for query based TLQ in DLS in this PR (+ added coverage for the V4 evaluator since that was missing so far).

cwperks · 2026-07-17T19:33:51Z

@cwperks but that's in DlsTermLookupQueryTest - or what are you looking for? that's a whole class just testing TLQ in DLS. and i've extended it with tests for query based TLQ in DLS in this PR (+ added coverage for the V4 evaluator since that was missing so far).

Ah, my bad. I'm glad we have tests in place for a number of scenarios already. At a glance, this seemed like an area that could have issues given that it can link to another index to query and the user running the query on the primary index may have restrictions on the second index. In that event, I wanted to make sure the restrictions applied, either if it is getting by id or doing a search to find terms to replace in the primary query.

rursprung · 2026-07-17T19:55:22Z

ah, my bad. now i see what you're looking for: the TLQ on another index which is itself protected by DLS. yes, i think that's not currently covered by tests.
but the way i understood it from @nibix (who was involved in the original TLQ-in-DLS implementation) the whole point of this is that the restrictions on the referenced index fall away for the DLS.

IIUC there is one caveat here (and i wasn't aware of this for a long time and i don't think that it's clearly documented?): the DLS is executed as a wrapper around the main query, i.e. this becomes one big query in the end. thus the restriction is also lifted for the wrapped query. though i haven't tried that yet.

since i'm off now i won't be able to update this PR until i get back. i leave it up to you if you want to merge this as-is or wait for additional test coverage.
from what i see:

without this query based TLQ-in-DLS isn't possible, so by merging this we're not changing existing (working) DLS queries and anyone starting to use it opts into it explicitly
the allowlisting behaviour is essentially the same as for existing id based TLQ-in-DLS, so again, no surprises there (beyond the fact that maybe already that behaviour is surprising?)

cwperks · 2026-07-19T12:35:09Z

@rursprung I merged this PR since its up to the cluster admin to write the DLS clause on each role, but I do think we should expand the coverage in this area. FYI DLS is adaptive by default. Take a look at

security/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java

Lines 247 to 269 in 28cc929

    
           if (mode == Mode.FILTER_LEVEL) { 
        
               doFilterLevelDls = true; 
        
               dlsRestrictionMap = config.getDocumentPrivileges().getRestrictions(context, resolved.local().names(context.clusterState())); 
        
           } else if (mode == Mode.LUCENE_LEVEL) { 
        
               doFilterLevelDls = false; 
        
           } else { // mode == Mode.ADAPTIVE 
        
               Mode modeByHeader = getDlsModeHeader(); 
        
               dlsRestrictionMap = config.getDocumentPrivileges().getRestrictions(context, resolved.local().names(context.clusterState())); 
        
               if (modeByHeader == Mode.FILTER_LEVEL) { 
        
                   doFilterLevelDls = true; 
        
                   log.debug("Doing filter-level DLS due to header"); 
        
               } else { 
        
                   doFilterLevelDls = dlsRestrictionMap.containsAny(DlsRestriction::containsTermLookupQuery); 
        
                   if (doFilterLevelDls) { 
        
                       setDlsModeHeader(Mode.FILTER_LEVEL); 
        
                       log.debug("Doing filter-level DLS because the query contains a TLQ"); 
        
                   } else { 
        
                       log.debug("Doing lucene-level DLS because the query does not contain a TLQ"); 
        
                   } 
        
               } 
        
           }

which controls the flow of what adaptive means. DLS can be at the filter level (modify the query) or applied on the Lucene level.

rursprung mentioned this pull request Jun 23, 2026

[FEATURE] support TLQ with query in DLS #6243

Closed

rursprung force-pushed the support-query-tlq-in-dls branch from faeba2f to ab9eb03 Compare June 24, 2026 14:10

rursprung force-pushed the support-query-tlq-in-dls branch from ab9eb03 to a8dcab5 Compare June 24, 2026 14:17

rursprung marked this pull request as ready for review June 24, 2026 14:18

rursprung requested review from DarshitChanpura, Rishav9852Kumar, RyanL1997, cwperks, derek-ho, finnegancarroll, nibix, reta, shikharj05 and willyborankin as code owners June 24, 2026 14:18

rursprung commented Jun 24, 2026

View reviewed changes

Comment thread ...main/java/org/opensearch/security/privileges/actionlevel/legacy/PrivilegesEvaluatorImpl.java

rursprung mentioned this pull request Jun 24, 2026

[BUG] missing test coverage for new evaluator V4 #6247

Open

reta reviewed Jun 26, 2026

View reviewed changes

Comment thread src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java Outdated

reta reviewed Jun 26, 2026

View reviewed changes

Comment thread src/main/java/org/opensearch/security/privileges/DocumentAllowList.java Outdated

rursprung force-pushed the support-query-tlq-in-dls branch from a8dcab5 to 55eb985 Compare June 29, 2026 11:53

rursprung force-pushed the support-query-tlq-in-dls branch from 55eb985 to dd84988 Compare June 29, 2026 11:57

rursprung requested a review from reta June 29, 2026 13:14

rursprung force-pushed the support-query-tlq-in-dls branch from dd84988 to d84e39e Compare July 9, 2026 16:20

reta reviewed Jul 9, 2026

View reviewed changes

Comment thread src/main/java/org/opensearch/security/privileges/DocumentAllowList.java

rursprung requested a review from reta July 10, 2026 08:55

reta reviewed Jul 10, 2026

View reviewed changes

Comment thread src/main/java/org/opensearch/security/privileges/DocumentAllowList.java Outdated

rursprung and others added 2 commits July 14, 2026 08:16

rursprung force-pushed the support-query-tlq-in-dls branch from d84e39e to 6a67442 Compare July 14, 2026 06:16

reta approved these changes Jul 14, 2026

View reviewed changes

cwperks merged commit 2310835 into opensearch-project:main Jul 19, 2026
113 of 114 checks passed

rursprung deleted the support-query-tlq-in-dls branch July 19, 2026 14:56

Uh oh!

Conversation

rursprung commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Testing

Check List

Uh oh!

github-actions Bot commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Analyzer ❗

Uh oh!

github-actions Bot commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit 6a67442)

Uh oh!

github-actions Bot commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions Bot commented Jun 24, 2026

Uh oh!

github-actions Bot commented Jun 24, 2026

Uh oh!

Uh oh!

rursprung commented Jun 24, 2026

Uh oh!

rursprung commented Jun 24, 2026

Uh oh!

rursprung commented Jun 25, 2026

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jun 29, 2026

Uh oh!

github-actions Bot commented Jun 29, 2026

Uh oh!

codecov Bot commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nibix commented Jun 29, 2026

Uh oh!

rursprung commented Jul 9, 2026

Uh oh!

github-actions Bot commented Jul 9, 2026

Uh oh!

Uh oh!

rursprung commented Jul 10, 2026

Uh oh!

Uh oh!

reta commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rursprung commented Jul 14, 2026

Uh oh!

github-actions Bot commented Jul 14, 2026

Uh oh!

reta commented Jul 14, 2026

Uh oh!

rursprung commented Jul 17, 2026

Uh oh!

reta commented Jul 17, 2026

Uh oh!

cwperks commented Jul 17, 2026

Uh oh!

cwperks commented Jul 17, 2026

Uh oh!

rursprung commented Jul 17, 2026

Uh oh!

cwperks commented Jul 17, 2026

Uh oh!

rursprung commented Jul 17, 2026

Uh oh!

Uh oh!

cwperks commented Jul 19, 2026

Uh oh!

rursprung commented Jun 23, 2026 •

edited

Loading

github-actions Bot commented Jun 23, 2026 •

edited

Loading

github-actions Bot commented Jun 23, 2026 •

edited

Loading

(Review updated until commit `6a67442`)

github-actions Bot commented Jun 23, 2026 •

edited

Loading

codecov Bot commented Jun 29, 2026 •

edited

Loading

reta commented Jul 10, 2026 •

edited

Loading